Skip to content

exception handling in slcManagement.js#1727

Open
munsakad wants to merge 23 commits into
developmentfrom
feitsopb_dee_exceptionhandling
Open

exception handling in slcManagement.js#1727
munsakad wants to merge 23 commits into
developmentfrom
feitsopb_dee_exceptionhandling

Conversation

@munsakad

Copy link
Copy Markdown
Contributor

Issue Description

Fixes #1535
-The withdraw function in slcManagement.js was silently swallowing errors, so users had no way of knowing when a course withdrawal failed.

Changes

Added a user-facing alert in the error handler of the withdraw ajax request so the user is notified when something goes wrong.
Fixed done to complete so the select reset actually runs after the request finishes.

Testing

Open the Manage Service Learning page.
Select "Withdraw" on a course to trigger the withdraw modal.
Click the withdraw button while the server is unreachable or returning an error.
Confirm that an alert appears informing the user that the withdrawal failed instead of failing silently.

BrianRamsay and others added 10 commits June 16, 2026 11:23
… page indicators

- slcNewProposal: restored Next/Submit to RIGHT column, Previous/Cancel/Exit on LEFT; dots now use flex centering on a full-width row so they stay straight at all screen sizes; flex-wrap on button groups prevents overflow
- emergencyContactInfo & insuranceInfo: responsive button rows now stack centered on mobile and sit on opposite ends on wider screens; buttons wrap cleanly instead of overflowing
Only slcNewProposal.html changes should be on this branch.
Separated the layout into two rows: top row holds Previous/Cancel/Exit
on the left and Next/Submit on the right (always on the same row at any
screen size), bottom row holds the centered step dots. This prevents the
dots from ever pushing right buttons below them when the page is scaled.
Cancel/Exit stays on the left, Save & Exit/Continue stays on the right.
Buttons use flex-wrap and responsive columns so they scale cleanly on all
screen sizes without overflow.
Replaces silent console.log with an alert so the user is notified when
a withdrawal fails. Also fixes done -> complete so the select reset
actually runs.
BhushanSah
BhushanSah previously approved these changes Jun 17, 2026
ArtemKurasov
ArtemKurasov previously approved these changes Jun 17, 2026

@ArtemKurasov ArtemKurasov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning seems to be displayed correctly

@zhytkovd zhytkovd self-requested a review June 17, 2026 15:07

@fritzj2 fritzj2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning appears to display correctly; however, it is confusing to read. Change it to an if/else statement for clarity. Check msgFlash() instead of alert for consistency across other files.

@brightfietsop-ux brightfietsop-ux dismissed stale reviews from ArtemKurasov and BhushanSah via d1551aa June 17, 2026 15:38

@zhytkovd zhytkovd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The msgFlash message shows under the pop-up window

fritzj2
fritzj2 previously approved these changes Jun 17, 2026

@fritzj2 fritzj2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make the code significantly easier to read and modify. The changes also allow the code to continue working as intended.

@MImran2002 MImran2002 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue has been fixed in the backend so all you would need to do is removed the console.log as the issue described because the error messaging is catching from the backend already so if there is an error it will be highlighted from the backend already. The request.response.message you implement can only work if the return from withdrawCourse return and error with a message only then the logic you wrote in the code will work.

So for now I would suggest you to remove the console.log.

BhushanSah
BhushanSah previously approved these changes Jun 17, 2026

@BhushanSah BhushanSah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this locally, and it works as expected.

The change improves the withdraw error handling by showing a user-facing flash message instead of only logging the error in the console. I also like that it checks for request.responseJSON.message and falls back to a clear default message when the server does not return one.

Using complete here also makes sense, since the dropdown reset should occur after the request finishes, whether it succeeds or fails.

No issues from my testing. Approved.

Comment thread app/static/js/slcManagement.js Outdated
Comment thread app/controllers/serviceLearning/routes.py Outdated
Comment thread app/static/js/slcManagement.js
Comment thread app/static/js/slcManagement.js Outdated
Comment thread app/static/js/slcManagement.js Outdated
@github-actions

Copy link
Copy Markdown

View Code Coverage

Comment on lines 205 to 211
if g.current_user.isAdmin or g.current_user.isFaculty:
withdrawProposal(courseID)
flash("Course successfully withdrawn", 'success')
return ""
else:
flash("Unauthorized to perform this action", 'warning')
except Exception as e:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are adding server codes return "", 500 should be nested in the exception or else and internal server outside of the exception doesn't make sense for me if exception is catching the errors. What do you think @BrianRamsay ? Should the other catches also consider 200 and 403?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle the exception in the ajax request in the withdraw function slcManagement.js file

8 participants